Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made communication identical in both directions. Removed modeling. Removed classes. #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Aaronius
Copy link

@Aaronius Aaronius commented Oct 23, 2016

Hello Dollar Shave Club! First of all, thanks for all the work you've put into this project. Your use of promises and closures is really smart. I have some feedback that I hope you'll consider. I won't be offended if you disagree or reject the PR outright. If you end up wanting to merge this PR, I'd be happy to update the docs (both the readme and code docs). Here are my thoughts:

  1. I believe mixing concerns of communication and modeling isn't a Good Thing™. I believe it would be better if Postmate were limited to just communication. If a consumer wants to back the communication with some sort of modeling then they should still be able to do so.
  2. I believe communication in both directions (parent-to-child, child-to-parent) should be identical and support passing data and returning a value.
  3. I don't believe classes provide value for what's trying to be accomplished. It's slightly awkward to me that newing the Postmate class results in a promise of a handshake negotiation. Classes also add some weight in babel overhead.

In this PR I've provided an alternative API that addresses the above points.

Here's how consumption is intended to work:

Parent:

import { connectParent } from 'postmate';

connectParent({
  container: document.getElementById('frame'),
  url: 'http://localhost:9000/child.html',
  methods: {
    // Methods exposed to the child.
    add(num1, num2) { return num1 + num2; }
  }
}).then(child => {
  child.multiply(2, 3).then(total => console.log(total));
});

Child:

import { connectChild } from 'postmate';

connectChild({
  methods: {
    // Methods exposed to the parent.
    multiply(num1, num2) { return num1 * num2; }
  }
}).then(parent => {
  parent.add(6, 2).then(total => console.log(total));
});

If a method must do something asynchronously before returning a value, the method may return a promise:

Child:

import { connectChild } from 'postmate';

connectChild({
  methods: {
    // Methods exposed to the parent.
    multiply(num1, num2) { return Promise.resolve(num1 * num2); }
  }
}).then(parent => {
  parent.add(6, 2).then(total => console.log(total));
});

With this design, call, on, emit, and get go away.

To change the Promise implementation:

import { setPromise } from 'postmate';

setPromise(RSVP.Promise);

To set debugging:

import { setDebug } from 'postmate';

setDebug(true);

You may notice that I changed the gulp code for building the library. Without the changes, I was getting a bunch of code from babel that wasn't providing value.

This PR drops the size of the lib by a KB.

@Aaronius
Copy link
Author

Hi there. Just checking in to see if you may have had time to look this over. My team could use (need) these changes pretty soon. Thanks!

@Aaronius
Copy link
Author

Due to the lack of response, I've assumed there isn't interest in these changes. Closing.

@Aaronius Aaronius closed this Oct 29, 2016
@jakiestfu
Copy link
Contributor

@Aaronius Hi! Didn't notice this PR until now! Let me review it and we will have an open discussion regarding this. Been very busy this week ;)

@jakiestfu jakiestfu reopened this Oct 29, 2016
@Aaronius
Copy link
Author

Aaronius commented Oct 29, 2016

Sorry, my team needed them quickly and since the entire API was changed I figured there wasn't much interest in trying to merge the changes. I polished them up, changed several other things, and released it as PenPal. This is awkward. :/

@jakiestfu
Copy link
Contributor

Will discuss more soon!

@jakiestfu
Copy link
Contributor

jakiestfu commented Oct 29, 2016

  1. What happens when I want to share properties that aren't methods? I.e. primitives or Promises directly?
new Postmate.Model({
  foo: 'bar',
  user: fetch('/user.json')
});
  1. If multiple instances of Postmate are running on the same page, does removing the constructor cause communication issues?
  2. Is there a need to use setPromise and setDebug as opposed to setting properties directly?

@Aaronius
Copy link
Author

Aaronius commented Nov 1, 2016

Regarding (1) you can expose a generic get method from the child if you would like:

Child

import { connectChild } from 'postmate';

const myModel = {
  foo: 'bar',
  user: fetch('/user.json')
};

connectChild({
  methods: {
    // Methods exposed to the parent.
    get(propName) { return myModel[propName];  }
  }
});

Parent

import { connectParent } from 'postmate';

connectParent({
  url: 'http://localhost:9000/child.html'
}).then(child => {
  child.get('foo').then(value => console.log(value));
  child.get('user').then(value => console.log(value));
});

Regarding (2), no, using a constructor doesn't provide any particular benefit here.

Regarding (3), in this PR I chose to use named exports instead of a default export. This is an es6 module thing (I'm not sure how familiar you are with es6 modules; this can help if you're not). If you import a primitive named export (e.g., import { debug } from 'penpal';) and then set it to a different value (e.g., debug = true) , the change doesn't "make it's way up" to the module the primitive came from (due to primitive variables being value-based and not reference-based). Anyway, long story short, in the end I decided to use a default export and you can set Promise and debug as you describe. I've made a number of changes since submitting this PR.

@jakiestfu
Copy link
Contributor

Thanks for the further explanation! There is a world in which I see merging these changes, but I have some personal issues with the differences between the two.

  1. I could see Postmate handling identical communication on both the parent and child
  2. We like the simplicity of exposing a model on each side as opposed to merely a set of methods. It makes it more difficult to implement things that postmate could easily handle. It's sole purpose is not to handle promises, rather, many types of data including promises.
  3. The function names connectParent and connectChild might change

What are your thoughts?

@Aaronius
Copy link
Author

Aaronius commented Nov 1, 2016

Regarding:

It makes it more difficult to implement things that postmate could easily handle. It's sole purpose is not to handle promises, rather, many types of data including promises.

I'd like to know more about what these things are. The way I've set it up, the get function in my previous example can also return many types of data including promises.

Regarding the name change, I've already changed them to connectToChild and connectToParent in PenPal.

@Andarist
Copy link
Contributor

@yowainwright @jakiestfu is there anything I could do to push this forward? Im interested in remote calls from child to parent, can dedicate to working on this.

@yowainwright
Copy link
Contributor

@Andarist yes!!! 🎉

We're planning on it. I've been working on updates (as you're aware of) to hopefully prevent regressions when we make the switch. I will try to get testing updates done ASAP for ya'!

@Andarist
Copy link
Contributor

@yowainwright if you need any help with the implementation or discussing the API (not sure if you want to release new version or do you want to avoid breaking changes) I would be happy to help/discuss :)

@yowainwright
Copy link
Contributor

@Andarist Yes, definitely! I will reach out to you soon.

@brucou
Copy link

brucou commented Dec 11, 2018

what is the status of that? Is it better to directly use penpal instead, with the anticipation that this will not be merged anytime soon?

@jakiestfu
Copy link
Contributor

jakiestfu commented Dec 12, 2018

@Andarist @brucou I’m going to revisit this very soon for a series of updates to Postmate, thanks for all of your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants